-
Notifications
You must be signed in to change notification settings - Fork 418
[Custom Transactions] Abstract dust-vs-nondust HTLC sorting away from channel #3921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
We discussed earlier passing the entire list of HTLCs to So we prefer to add a single method that surfaces this limit to channel, and then let channel sort HTLCs depending on whether their value sits above or below that amount. As a result, builders of custom transactions will only have a say on where the dust limit sits, and won't be able to choose arbitrary subsets for dust and non-dust HTLCs. |
Is the main motivation for this to get all of the commitment-related logic out of
The key questions we're looking to answer seems to be "can I afford a commitment with this theoretical dust/nondust htlc"? This does seem to be something we could move into Sadly here's no getting around needing to know the dust limit if we want to clamp our capacity to that value, so we'd still need to surface
Tempting to suggest just adding an so tl;dr: I'd be interested in seeing what trying to pull more of the dust logic out into Meta note: This has got a lot of overlap with 3bb0586, so I think we should either:
|
I would say we focus on the latter for this PR, and let the former be the overarching goal :)
Let me know what you think of this new direction here. Still have some clunkiness to resolve, but that's what it's looking like right now.
|
Definitely let me rebase on top of your PR, you've rebased once already :) |
12fd6c3
to
067bfa0
Compare
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
I like this approach! Definitely prefer being able to move a lot of the dust / second stage tx reasoning into the builder 👍 |
🔔 2nd Reminder Hey @carlaKC! This PR has been waiting for your review. |
Move these decisions to `TxBuilder` when calculating total dust exposure and total transaction fees. We would have liked to delete `TxBuilder::htlc_success_timeout_dust_limits`, but this is required in `get_available_balances` to potentially clamp the balance to the dust limit. Because this method is there, we also use it to determine whether a single HTLC is dust or nondust (otherwise we would have used a `TxBuilder::is_dust` call on that HTLC). Along the way, we also sort non-dust HTLCs in the holding cell using the exact proposed feerate in `can_send_update_fee`. When checking whether we can afford the next commitment transaction fee, we should not count HTLCs in the holding cell using the dust buffer feerate, as this can trim HTLCs that will be non-dust on the next commitment transaction. Rather we should use the exact proposed feerate; this ensures we account for all non-dust HTLCs on the next commitment transaction.
I'm aware I still owe a follow-up PR from the previous PR in this project, will push that soon in a separate PR :) |
Uh oh!
There was an error while loading. Please reload this page.